lightning-block-sync: switch to bitreq, drop chunked_transfer #4350
Conversation
|
👋 I see @tnull was un-assigned. |
|
@TheBlueMatt the ci fails due to the problem in payment/bolt11.rs in ldk-node, which is unrelated to this task, should i open a pr to fix this in ldk-node as i have made the changes locally while testing. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks for taking a look, had a quick first look, here are some initial remarks.
lightning-block-sync/Cargo.toml
Outdated
| rpc-client = [ "serde_json", "chunked_transfer" ] | ||
| rest-client = [ "serde_json", "bitreq" ] | ||
| rpc-client = [ "serde_json", "bitreq" ] | ||
| tokio = [ "dep:tokio", "bitreq/async" ] |
There was a problem hiding this comment.
I'm confused: why does tokio depend on bitreq?
There was a problem hiding this comment.
changing this to bitreq?/async this is because the rest and rpc client enables the sync mode of bitreq by default so this would enable the async mode of bitreq which uses send_async_with_client() if bitreq exists.
lightning-block-sync/src/http.rs
Outdated
| /// Server for handling HTTP client requests with a stock response. | ||
| pub struct HttpServer { | ||
| address: std::net::SocketAddr, | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Why do we need to allow dead code here?
There was a problem hiding this comment.
it was causing a warning so i kept that before, though now I've Implemented Drop for HttpServer that will use all the fields.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4350 +/- ##
==========================================
- Coverage 86.09% 86.06% -0.03%
==========================================
Files 156 156
Lines 102804 102708 -96
Branches 102804 102708 -96
==========================================
- Hits 88508 88397 -111
- Misses 11787 11819 +32
+ Partials 2509 2492 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -572,27 +334,54 @@ mod endpoint_tests { | |||
| #[cfg(test)] | |||
| pub(crate) mod client_tests { | |||
There was a problem hiding this comment.
I'm not convinced that any of the remaining tests in this file test anything useful now.
There was a problem hiding this comment.
This wasn't addressed. Do you agree? Are there some tests you think are useful here?
There was a problem hiding this comment.
I'm unsure on what else can be tested in this, there isn't any heavy stuff that we can test here, so if you can suggest any then i'll add those.
There was a problem hiding this comment.
If there's tests that aren't testing anything useful, we should remove them.
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
lightning-block-sync/src/http.rs
Outdated
| .with_header("Host", host) | ||
| .with_header("Connection", "keep-alive") |
There was a problem hiding this comment.
please let bitreq handle this
lightning-block-sync/src/http.rs
Outdated
| .with_max_headers_size(Some(MAX_HTTP_MESSAGE_HEADER_SIZE)) | ||
| .with_max_status_line_length(Some(MAX_HTTP_MESSAGE_HEADER_SIZE)) |
There was a problem hiding this comment.
Please let bitreq handle this
lightning-block-sync/src/http.rs
Outdated
| Some(address) => address, | ||
| }; | ||
|
|
||
| // Verify reachability by attempting a connection. |
There was a problem hiding this comment.
If you're gonna verify, please make a request
lightning-block-sync/src/http.rs
Outdated
| } | ||
|
|
||
| /// Converts a bitreq error to an std::io::Error. | ||
| fn bitreq_to_io_error(err: bitreq::Error) -> std::io::Error { |
There was a problem hiding this comment.
Please just change the API?
There was a problem hiding this comment.
doing that would mean that we will have to make changes in the ldk-node as well, so should i do that?
There was a problem hiding this comment.
Yes, that would be much appreciated.
| @@ -572,27 +334,54 @@ mod endpoint_tests { | |||
| #[cfg(test)] | |||
| pub(crate) mod client_tests { | |||
There was a problem hiding this comment.
This wasn't addressed. Do you agree? Are there some tests you think are useful here?
Apostlex0
force-pushed
the
feat/blocks-sync-bitreq
branch
from
8abe808 to
16677e7
Compare
4 participants
fixes #4325
the ldk-node tests doesn't seem to need any changes as the changes here preserve the public api, hence what happens internally doesn't really matter as all the existing tests pass without any problems.
Though changes to payment/bolt11.rs were needed to compile and run the tests.